Fix issue #27, world age problem on v0.6 #30
Conversation
|
Would be preferable if you could avoid copy-pasting a refactored copy of a long function here. Please make the change as minimal as possible to fix the issue. |
|
That VERSION cutoff isn't accurate either. If the 0.6 code doesn't work on prior versions of Julia, better to just make the package 0.6-only, drop all the Compat cruft. If bug fixes for 0.4 or 0.5 are needed they could be made off a branch. |
|
I suggest we should merge without worry about refactoring if it fixes a known issue, and have a new issue for refactoring. Of course the VERSION stuff should be done right. |
src/cformat.jl
Outdated
| @@ -1,16 +1,80 @@ | |||
| formatters = Dict{ Compat.ASCIIString, Function }() | |||
|
|
|||
| function sprintf1( fmt::Compat.ASCIIString, x ) | |||
| @static if VERSION >= v"0.6.0-dev.1671" | |||
There was a problem hiding this comment.
is static absolutely needed here?
There was a problem hiding this comment.
I'd believed it was the preferred way to deal with sections of code that should not be compiled because of platform or version differences.
Is that not the case?
There was a problem hiding this comment.
It's completely unnecessary at top-level
There was a problem hiding this comment.
OK, that's good to know (it might be nice to have the docstring for @static mention that)
In my own code, I'd just replaced any of the old macros (@windows etc. with @static if ... everwhere, and didn't realize that the @static part was not needed at the top level (does it hurt anything though to leave it in?)
There was a problem hiding this comment.
it means code in the not evaluated conditional won't trigger warnings or some errors, which sometimes is useful but other times is like ifdefs in c, can hide issues if you only ever develop on systems where one of the branches is true and never the other
There was a problem hiding this comment.
OK, that makes sense, I've removed the @static
There was a problem hiding this comment.
Although, in these cases where I've used it to distinguish between v0.5.x and v0.6, both branches are being tested and used (with Travis-CI on GitHub, and at work, where even though we are deploying now on v0.5.1, I'm making sure that we'll be ready for v0.6 as soon as it is released and all of the packages that we use have been updated to not have any deprecations)
|
If duplicating code isn't needed to fix the problem, then this shouldn't be duplicating code. |
|
I've changed to use the exact version number of when JuliaLang/julia#17057 was merged. |
|
I didn't have the time to rewrite things to make just the minimal changes in order to make this work for the old Formatting.jl package. Somebody had asked me nicely if I could backport my fixes for v0.6 to the old package, hence this PR. |
|
Can this be merged cc @tkelman |
|
would be better to drop 0.4 and 0.5 and make this 0.6-only than duplicate everything |
|
Yeah ok, there are a lot of thing that could be better in this world. This package working on 0.6 is better than this package not working. |
|
fine, I'll do it then. I didn't have anything better to do at all than fix my own review request from January. |
Again, trying to deal with Travis-CI issues, same change as before.